Conversation
…t to OrderedDict. See #PARAF-345
📝 WalkthroughWalkthroughFixes session-shape regressions: introduces a readonly-aware Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/esign/browser/views.py (1)
223-226: Cache the invalidsession_idfallback too (minor optimization).On Line 226,
{}is returned without setting_cached_session, so repeated template/property accesses redo parsing.Suggested small diff
try: session_id = int(session_id) except (TypeError, ValueError): - return {} + self._cached_session = {} + return self._cached_session🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/views.py` around lines 223 - 226, The code returns an empty dict on invalid session_id but doesn't set the cached value, causing repeated parsing; update the exception branch in the session_id parsing block (where session_id = int(session_id) is tried) to assign self._cached_session = {} (or the appropriate cache container) before returning so subsequent accesses use the cached fallback; ensure this change is applied in the method that uses self._cached_session in views.py so the invalid-session path is cached.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/esign/browser/views.py`:
- Around line 223-226: The code returns an empty dict on invalid session_id but
doesn't set the cached value, causing repeated parsing; update the exception
branch in the session_id parsing block (where session_id = int(session_id) is
tried) to assign self._cached_session = {} (or the appropriate cache container)
before returning so subsequent accesses use the cached fallback; ensure this
change is applied in the method that uses self._cached_session in views.py so
the invalid-session path is cached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f24efc6-e843-4960-ae15-2b1157c74d0a
📒 Files selected for processing (3)
CHANGES.rstsrc/imio/esign/browser/views.pysrc/imio/esign/utils.py
…this way, change FacetedSessionInfoViewlet.sessions
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/imio/esign/browser/views.py`:
- Around line 219-230: The code builds and caches a mapping {session_id:
get_session_info(session_id)} without checking whether get_session_info returned
a valid session, causing a truthy empty-mapping to be returned; update the logic
in the view method that uses self._cached_session and session_id to call
get_session_info(session_id), check if the returned session is truthy (not
None/empty), and only then set and return {session_id: session}; if
get_session_info returns falsy, return {} (and avoid caching an invalid
mapping).
In `@src/imio/esign/utils.py`:
- Around line 334-347: The change made get_session_info(session_id, portal=None,
readonly=True) to deepcopy sessions by default which alters shared utility
semantics; revert the default readonly to False in get_session_info and ensure
deepcopy is only performed when readonly is explicitly True (keep usage of
deepcopy inside get_session_info guarded by the readonly flag), and move any
read-only deepcopy usage from callers (e.g., the viewlet) into the viewlet code
so callers that expect the live session mapping continue to receive the original
mapping returned by get_session_annotation and the session variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01406bb4-850b-414e-9908-78043e88ccbb
📒 Files selected for processing (2)
src/imio/esign/browser/views.pysrc/imio/esign/utils.py
| def get_session_info(session_id, portal=None, readonly=True): | ||
| """Return a session info for a given numbering. | ||
|
|
||
| :param session_id: the session id to return | ||
| :param portal: portal if necessary to get the session annotation | ||
| :param readonly: return a copy of stored data to avoid modifying it | ||
| """ | ||
| annot = get_session_annotation(portal=portal) | ||
| session = {} | ||
| if session_id in annot['sessions']: | ||
| return annot['sessions'][session_id] | ||
| session = annot['sessions'][session_id] | ||
| if readonly: | ||
| session = deepcopy(session) | ||
| return session |
There was a problem hiding this comment.
Avoid changing get_session_info semantics in this PR.
This introduces a default deepcopy behavior in a shared utility, which is broader than the PR objective and can break callers expecting a live session mapping.
💡 Suggested adjustment
-def get_session_info(session_id, portal=None, readonly=True):
+def get_session_info(session_id, portal=None):
@@
- if session_id in annot['sessions']:
- session = annot['sessions'][session_id]
- if readonly:
- session = deepcopy(session)
+ if session_id in annot['sessions']:
+ session = annot['sessions'][session_id]
return sessionIf read-only behavior is needed for the viewlet, apply deepcopy in the viewlet only.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_session_info(session_id, portal=None, readonly=True): | |
| """Return a session info for a given numbering. | |
| :param session_id: the session id to return | |
| :param portal: portal if necessary to get the session annotation | |
| :param readonly: return a copy of stored data to avoid modifying it | |
| """ | |
| annot = get_session_annotation(portal=portal) | |
| session = {} | |
| if session_id in annot['sessions']: | |
| return annot['sessions'][session_id] | |
| session = annot['sessions'][session_id] | |
| if readonly: | |
| session = deepcopy(session) | |
| return session | |
| def get_session_info(session_id, portal=None): | |
| """Return a session info for a given numbering. | |
| :param session_id: the session id to return | |
| :param portal: portal if necessary to get the session annotation | |
| """ | |
| annot = get_session_annotation(portal=portal) | |
| session = {} | |
| if session_id in annot['sessions']: | |
| session = annot['sessions'][session_id] | |
| return session |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/imio/esign/utils.py` around lines 334 - 347, The change made
get_session_info(session_id, portal=None, readonly=True) to deepcopy sessions by
default which alters shared utility semantics; revert the default readonly to
False in get_session_info and ensure deepcopy is only performed when readonly is
explicitly True (keep usage of deepcopy inside get_session_info guarded by the
readonly flag), and move any read-only deepcopy usage from callers (e.g., the
viewlet) into the viewlet code so callers that expect the live session mapping
continue to receive the original mapping returned by get_session_annotation and
the session variable.
|
@chris-adam juste le fix pour le viewlet dans faceted, j'ai aussi ajouté le readonly sur utils.get_session_info "à la" utils.get_sessions_for et ajouté du cache soft vu que la méthode est appelée 2 fois, attention avec @Property, c'est calculé à chaque fois qu'on l'appelle comme une méthode, donc parfois on peut mettre du cache, en cachant localement dans un attr sur le viewlet, c'est non persistant (db) et très rapide car "ram.cache", est parfois + lent que le code qui est exécuté |
… found, so we can iterate on it instead returning None that must be managed specifically
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/esign/browser/views.py (1)
224-225: Cache the invalid-input branch for consistent soft-cache behavior.Line 225 returns
{}without populating_cached_session, so repeated property access still reparses input. Consider caching{}here too for consistency with the rest of this method.♻️ Suggested patch
try: session_id = int(session_id) except (TypeError, ValueError): - return {} + self._cached_session = {} + return self._cached_session🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/imio/esign/browser/views.py` around lines 224 - 225, The except branch returns {} without storing it in the cache, so repeated accesses re-run parsing; update the except (TypeError, ValueError) block in the same function/property that uses _cached_session to assign _cached_session = {} before returning so the empty result is cached consistently (refer to the _cached_session variable and the method/property that previously sets and returns it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/imio/esign/browser/views.py`:
- Around line 224-225: The except branch returns {} without storing it in the
cache, so repeated accesses re-run parsing; update the except (TypeError,
ValueError) block in the same function/property that uses _cached_session to
assign _cached_session = {} before returning so the empty result is cached
consistently (refer to the _cached_session variable and the method/property that
previously sets and returns it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4612c74-5b42-4c7f-90d6-0cc868fa9d59
📒 Files selected for processing (1)
src/imio/esign/browser/views.py
@gbastien Ok, et qu'est-ce que tu penses du décorateur cachedproperty ? |
2 participants
See #PARAF-345
Summary by CodeRabbit
Bug Fixes
Documentation